Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui(intsaller): ping electrum backend both with tcp & ssl #1296

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pythcoiner
Copy link
Collaborator

This PR add a double ping (both tcp & ssl) when defining an electrum server address.
After ping if the prefix is not ssl we display a warning to the user:
image

@nondiremanuel
Copy link
Collaborator

I won't comment on the technical solution, but on the UI side I would use yellow/orange (the usual warning color) for warnings. I would possibly make it a little bigger

@pythcoiner
Copy link
Collaborator Author

  • the size is the usual size we use for warning/error on not valid field, so i think we should keep this one
  • will change the color

@pythcoiner
Copy link
Collaborator Author

image
image

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Sep 10, 2024

Do we need to warn about SSL if people are using on their local network?

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Sep 10, 2024

Wouldn't it be simpler to have a "Use SSL" checkbox?

@pythcoiner
Copy link
Collaborator Author

the advantage of ping both is i guess almost the time our user will not know what is ssl, so w/ this solution they don't have to care about.

@pythcoiner
Copy link
Collaborator Author

Do we need to warn about SSL if people are using on their local network?

i think it's not easy to be sure wether we are a local network or not

@darosior darosior removed this from the v7 - Liana milestone Sep 10, 2024
@nondiremanuel nondiremanuel added this to the v8 - Liana milestone Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants